Skip to content

schemachanger: add error-on-fallthrough guards on vistors#169359

Merged
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
bghal:schemachanger-error-fallthrough
May 1, 2026
Merged

schemachanger: add error-on-fallthrough guards on vistors#169359
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
bghal:schemachanger-error-fallthrough

Conversation

@bghal
Copy link
Copy Markdown
Contributor

@bghal bghal commented Apr 29, 2026

The issue in #169345 demonstrated the risk of switch statements on types
if an unexpected type is passed to the vistor function. This change
guards against future bugs by adding errors-on-default that trip on
unexpected types.

Epic: CRDB-31325

Informed by: #169345

Release note: None

@trunk-io
Copy link
Copy Markdown
Contributor

trunk-io Bot commented Apr 29, 2026

😎 Merged successfully - details.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 29, 2026

Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link)

@bghal bghal force-pushed the schemachanger-error-fallthrough branch from 6b497b8 to 9796f77 Compare April 30, 2026 15:57
@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 30, 2026

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@bghal bghal marked this pull request as ready for review April 30, 2026 20:54
@bghal bghal requested a review from a team as a code owner April 30, 2026 20:54
Copy link
Copy Markdown
Contributor

@spilchen spilchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bghal bghal force-pushed the schemachanger-error-fallthrough branch from 9796f77 to 4f8068b Compare May 1, 2026 15:08
The issue in cockroachdb#169345 demonstrated the risk of switch statements on types
if an unexpected type is passed to the vistor function. This change
guards against future bugs by adding errors-on-default that trip on
unexpected types.

Epic: CRDB-31325

Informed by: cockroachdb#169345

Release note: None
@bghal bghal force-pushed the schemachanger-error-fallthrough branch from 4f8068b to cd36d28 Compare May 1, 2026 15:13
@cockroach-teamcity
Copy link
Copy Markdown
Member

⚪ Sysbench [SQL, 3node, oltp_read_write]
Metric Old Commit New Commit Delta Note
sec/op 10.19m ±1% 10.15m ±1% ~ p=0.539 n=15
allocs/op 8.123k ±0% 8.144k ±0% ~ p=0.300 n=15
Reproduce

benchdiff binaries:

mkdir -p benchdiff/cd36d28/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/cd36d28b77d439fd762a6795ca1fa90a4128d2d0/bin/pkg_sql_tests benchdiff/cd36d28/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/cd36d28/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/bca92f3/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/bca92f3b0e140f41ea2911e939d698e68eae8b06/bin/pkg_sql_tests benchdiff/bca92f3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/bca92f3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=bca92f3 --new=cd36d28 --memprofile ./pkg/sql/tests
⚪ Sysbench [KV, 3node, oltp_read_only]
Metric Old Commit New Commit Delta Note
sec/op 3.234m ±2% 3.294m ±1% ~ p=0.029 n=15
allocs/op 2.102k ±0% 2.102k ±0% ~ p=0.883 n=15
Reproduce

benchdiff binaries:

mkdir -p benchdiff/cd36d28/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/cd36d28b77d439fd762a6795ca1fa90a4128d2d0/bin/pkg_sql_tests benchdiff/cd36d28/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/cd36d28/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/bca92f3/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/bca92f3b0e140f41ea2911e939d698e68eae8b06/bin/pkg_sql_tests benchdiff/bca92f3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/bca92f3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=bca92f3 --new=cd36d28 --memprofile ./pkg/sql/tests
🔴 Sysbench [KV, 3node, oltp_write_only]
Metric Old Commit New Commit Delta Note
🔴 sec/op 3.079m ±3% 3.199m ±3% +3.88% p=0.006 n=15
allocs/op 4.218k ±0% 4.221k ±0% ~ p=0.382 n=15
Reproduce

benchdiff binaries:

mkdir -p benchdiff/cd36d28/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/cd36d28b77d439fd762a6795ca1fa90a4128d2d0/bin/pkg_sql_tests benchdiff/cd36d28/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/cd36d28/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/bca92f3/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/bca92f3b0e140f41ea2911e939d698e68eae8b06/bin/pkg_sql_tests benchdiff/bca92f3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/bca92f3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=bca92f3 --new=cd36d28 --memprofile ./pkg/sql/tests
Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/cd36d28b77d439fd762a6795ca1fa90a4128d2d0/25219761023-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/bca92f3b0e140f41ea2911e939d698e68eae8b06/25219761023-1/\* old/

built with commit: cd36d28b77d439fd762a6795ca1fa90a4128d2d0

@cockroach-teamcity cockroach-teamcity added the X-perf-check Microbenchmarks CI: Added to a PR if a performance regression is detected and should be checked label May 1, 2026
@bghal
Copy link
Copy Markdown
Contributor Author

bghal commented May 1, 2026

/trunk merge

@trunk-io trunk-io Bot merged commit b60b234 into cockroachdb:master May 1, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

target-release-26.3.0 X-perf-check Microbenchmarks CI: Added to a PR if a performance regression is detected and should be checked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants